feat: add format script + optional git hooks#2579
Merged
Conversation
github-actions
Bot
added
studio
monorepo
controlplane
cli
router
graphqlmetrics
aws-lambda-router
labels
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Husky hooks and adjusts .gitignore; introduces per-package and root format scripts plus Makefile targets; registers four new workspace packages; standardizes formatting scripts across many packages; applies widespread formatting, minor test/schema syntax fixes, and email/template reflows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2579 +/- ##
==========================================
+ Coverage 37.21% 43.90% +6.69%
==========================================
Files 967 1059 +92
Lines 127082 148150 +21068
Branches 5223 9412 +4189
==========================================
+ Hits 47288 65050 +17762
- Misses 78148 81334 +3186
- Partials 1646 1766 +120
🚀 New features to boost your workflow:
|
Router image scan passed✅ No security vulnerabilities found in image: |
comatory
force-pushed
the
ondrej/formatter
branch
from
a2e7f17 to
be30482
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
playground/package.json (1)
37-37: Split write and check into separate scripts.Prettier v3 recommends using
--writefor development (to apply formatting) and--checkfor CI (to verify without modifying). Line 37 combines both, which undermines the semantic intent. Theformatscript should write-only, and a separateformat:checkscript (reusable in CI) enables proper build verification.♻️ Proposed change
- "format": "prettier -w -c ." + "format": "prettier -w .", + "format:check": "prettier -c ."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playground/package.json` at line 37, The package.json "format" script currently mixes write and check flags ("-w -c"), so change the "format" script to use the write-only flag (prettier --write <same-glob>) and add a new "format:check" script that runs the check-only flag (prettier --check <same-glob>); update the script names "format" and add "format:check" accordingly to separate developer formatting (write) from CI verification (check).CONTRIBUTING.md (2)
68-69: Clarify hook wording (pre-commitvscommit-msg).The sentence currently says “pre-commit hook” but also mentions commit message validation. Consider “git hooks” to avoid ambiguity.
✍️ Suggested wording
-If you want to enforce this standard, you can set up a pre-commit hook. This functionality is provided by [husky](https://typicode.github.io/husky/#/). Run `pnpm husky` to install pre-commit hooks which format code and check commit message format. +If you want to enforce this standard, you can set up git hooks. This functionality is provided by [husky](https://typicode.github.io/husky/#/). Run `pnpm husky` to install hooks that format code and validate commit message format.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` around lines 68 - 69, Update the ambiguous phrase "pre-commit hook" to a clearer term like "git hooks" and explicitly mention the two relevant hook types (pre-commit for formatting and commit-msg for commit message validation) in the CONTRIBUTING.md text that currently references husky and the husky.sh file; ensure the sentence reads something like "Run `pnpm husky` to install git hooks (pre-commit for formatting and commit-msg for commit message validation)" so readers understand both behaviors and where husky.sh fits.
179-181: Minor wording polish for readability and naming consistency.Use “its” (not “it’s”) and capitalize “GitHub” consistently.
✍️ Suggested wording
-- Upon merge we find the PR and it's HEAD commit using the GitHub rest API -- We find the github artifact from this commit hash +- Upon merge we find the PR and its HEAD commit using the GitHub REST API +- We find the GitHub artifact for this commit hash🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` around lines 179 - 181, Change the three phrases in the CONTRIBUTING.md snippet to use correct possessive and consistent capitalization: replace "it's HEAD commit" with "its HEAD commit", "github artifact" with "GitHub artifact", and ensure any other "github" instances in the same paragraph are capitalized as "GitHub" for consistency.Makefile (1)
58-71: Prefer a namedpackagevariable overMAKECMDGOALSparsing.This works, but it’s fragile and can collide with real targets when package names overlap. A named argument removes the need for dynamic no-op targets and is easier to maintain.
♻️ Suggested simplification
+ .PHONY: format format-all + format-all: pnpm -r --parallel format format: - `@package`="$(word 2,$(MAKECMDGOALS))"; \ - if [ -z "$$package" ]; then \ - echo "Usage: make format <package>"; \ + `@if` [ -z "$(package)" ]; then \ + echo "Usage: make format package=<package>"; \ exit 1; \ fi; \ - pnpm --filter "./$$package" --fail-if-no-match run format - -ifneq ($(filter format,$(MAKECMDGOALS)),) -FORMAT_ARGS := $(wordlist 2,$(words $(MAKECMDGOALS)),$(MAKECMDGOALS)) -.PHONY: $(FORMAT_ARGS) -$(FORMAT_ARGS): - @: -endif + pnpm --filter "./$(package)" --fail-if-no-match run format🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 58 - 71, The current format target parses MAKECMDGOALS and generates dynamic no-op targets (FORMAT_ARGS) which is fragile; change the rule to accept an explicit package variable (e.g., package=<name>) and remove the dynamic TARGET generation and FORMAT_ARGS logic. Update the format target/recipe (the existing format: target and the block that uses MAKECMDGOALS) to validate that the package variable is provided, print a usage message and exit if missing, then run pnpm --filter "./$(package)" --fail-if-no-match run format; remove the ifneq ($(filter format,$(MAKECMDGOALS)),...) block and any references to FORMAT_ARGS so callers use “make format package=your-package” instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client-tests/apollo-js/package.json`:
- Line 7: The "format" npm script in package.json currently combines Prettier's
write and check flags ("prettier -w -c ."), which conflicts; update the "format"
script (the "format" entry in package.json) to only run Prettier in write mode
by removing the check flag (e.g., use "prettier -w ." or "prettier --write .")
so formatting is applied correctly.
In `@package.json`:
- Line 26: Fix the typo in the package.json "format" npm script: change the
misspelled pnpm flag "--parlallel" to the correct "--parallel" in the "format"
script so pnpm run -r --parallel format executes correctly across packages;
update the value of the "format" script string accordingly.
In `@studio/package.json`:
- Around line 20-21: The "format" npm script currently uses both --write (-w)
and --check (-c) flags which conflict; update the "format" script entry named
"format" (currently "prettier -w -c .") to remove the --check/-c flag so it
becomes a pure write command (e.g., "prettier -w ."), leaving "lint:fix"
unchanged.
---
Nitpick comments:
In `@CONTRIBUTING.md`:
- Around line 68-69: Update the ambiguous phrase "pre-commit hook" to a clearer
term like "git hooks" and explicitly mention the two relevant hook types
(pre-commit for formatting and commit-msg for commit message validation) in the
CONTRIBUTING.md text that currently references husky and the husky.sh file;
ensure the sentence reads something like "Run `pnpm husky` to install git hooks
(pre-commit for formatting and commit-msg for commit message validation)" so
readers understand both behaviors and where husky.sh fits.
- Around line 179-181: Change the three phrases in the CONTRIBUTING.md snippet
to use correct possessive and consistent capitalization: replace "it's HEAD
commit" with "its HEAD commit", "github artifact" with "GitHub artifact", and
ensure any other "github" instances in the same paragraph are capitalized as
"GitHub" for consistency.
In `@Makefile`:
- Around line 58-71: The current format target parses MAKECMDGOALS and generates
dynamic no-op targets (FORMAT_ARGS) which is fragile; change the rule to accept
an explicit package variable (e.g., package=<name>) and remove the dynamic
TARGET generation and FORMAT_ARGS logic. Update the format target/recipe (the
existing format: target and the block that uses MAKECMDGOALS) to validate that
the package variable is provided, print a usage message and exit if missing,
then run pnpm --filter "./$(package)" --fail-if-no-match run format; remove the
ifneq ($(filter format,$(MAKECMDGOALS)),...) block and any references to
FORMAT_ARGS so callers use “make format package=your-package” instead.
In `@playground/package.json`:
- Line 37: The package.json "format" script currently mixes write and check
flags ("-w -c"), so change the "format" script to use the write-only flag
(prettier --write <same-glob>) and add a new "format:check" script that runs the
check-only flag (prettier --check <same-glob>); update the script names "format"
and add "format:check" accordingly to separate developer formatting (write) from
CI verification (check).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.gitignore.husky/commit-msg.husky/pre-commitCONTRIBUTING.mdMakefileadmission-server/package.jsonaws-lambda-router/package.jsoncdn-server/package.jsoncli/package.jsonclient-tests/apollo-js/package.jsoncomposition/package.jsoncontrolplane/.prettierignorecontrolplane/package.jsongraphqlmetrics/package.jsonpackage.jsonplayground/package.jsonpnpm-workspace.yamlprotographic/package.jsonrouter-plugin/package.jsonrouter/package.jsonshared/package.jsonstudio/package.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
cli/package.json (1)
31-32:⚠️ Potential issue | 🟠 Major
lint:fixnow depends on a conflictingformatcommand.
Please switchformatto write-only mode.Proposed fix
- "format": "prettier -w -c .", + "format": "prettier --write .",In Prettier CLI v3, what is the documented behavior when both --write and --check are passed in the same command?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/package.json` around lines 31 - 32, The "format" npm script currently mixes write and check flags ("prettier -w -c ."), which conflicts when invoked from "lint:fix"; update the "format" script to write-only by removing the check flag so it becomes a pure write operation (keep the -w/--write and the target glob), ensuring "lint:fix" can run "pnpm format" without flag conflicts; target the "format" script entry in package.json and modify it accordingly.playground/package.json (1)
37-37:⚠️ Potential issue | 🟠 MajorUse write-only mode in
format.
Same flag conflict pattern as above (-w+-c).Proposed fix
- "format": "prettier -w -c ." + "format": "prettier --write ."In Prettier CLI v3, what is the documented behavior when both --write and --check are passed in the same command?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playground/package.json` at line 37, The "format" npm script currently uses conflicting Prettier flags ("-w" and "-c") in the "format" script entry; change it to a write-only command by removing the check flag (use "prettier -w ." or the equivalent long form "--write .") or split into two scripts (e.g., "format" with --write and "format:check" with --check) so "format" no longer passes both --write and --check simultaneously; update the "format" script name in package.json accordingly.shared/package.json (1)
22-24:⚠️ Potential issue | 🟠 MajorUse write-only mode for
formatscript.
lint:fixnow delegates topnpm format, soformatshould be deterministic write mode. Mixing-wand-cis risky and can break/fight the intent of auto-fix.Proposed fix
- "format": "prettier -w -c .", + "format": "prettier --write .",In Prettier CLI v3, what is the documented behavior when both --write and --check are passed in the same command?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/package.json` around lines 22 - 24, The format script currently mixes write and check flags; update the "format" npm script referenced by "lint:fix" to run Prettier in write-only mode (remove the check flag) so formatting is deterministic—replace the current "prettier -w -c ." invocation with a write-only invocation (e.g., use --write or -w only) in package.json's "format" script and keep "lint:fix" delegating to "pnpm format".composition/package.json (1)
23-24:⚠️ Potential issue | 🟠 Major
lint:fixshould call a write-only formatter.
pnpm formatcurrently usesprettier -w -c .; please remove check mode from the fix path.Proposed fix
- "format": "prettier -w -c .", + "format": "prettier --write .",In Prettier CLI v3, what is the documented behavior when both --write and --check are passed in the same command?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/package.json` around lines 23 - 24, The "lint:fix" script currently calls "pnpm format" which maps to the "format" script using "prettier -w -c .", so fix mode is being mixed with check mode; update the "format" script to remove the check flag (change "prettier -w -c ." to "prettier -w .") so "lint:fix" runs a write-only formatter, or alternatively change "lint:fix" to directly call a write-only Prettier (e.g., "prettier -w ."); optionally add a separate "format:check" script if you still need a check-only command.
🧹 Nitpick comments (3)
CONTRIBUTING.md (1)
69-69: Tighten grammar in setup note.Line 69 reads “In some setup,” which is awkward. Use “In some setups” (or “In some environments”) for clarity.
Proposed wording
-In some setup, you have to tell husky where to find your package manager or binaries. +In some setups, you have to tell husky where to find your package manager or binaries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` at line 69, Update the sentence in CONTRIBUTING.md to improve grammar by replacing “In some setup,” with a clearer phrase such as “In some setups,” (or “In some environments,”) so the note reads: “In some setups, you have to tell husky where to find your package manager or binaries.” Locate the sentence in the husky installation note and apply the replacement to ensure subject-verb agreement and clarity.package.json (1)
40-42: The case pattern doesn't exclude top-levelgen/orvendor/files, though this is not currently an issue in the repository.
The current pattern*/gen/*|*/vendor/*correctly skips generated files in nested directories (e.g.,./connect-go/gen/...), but would miss top-levelgen/orvendor/files if they existed. Since the repository has no top-levelgen/orvendor/directories, the pattern works as intended in practice. However, for defensive robustness, consider including top-level matches:Proposed fix
- "sh -c 'for file in \"$@\"; do case \"$file\" in */gen/*|*/vendor/*) ;; *) gofmt -w \"$file\" ;; esac; done' --" + "sh -c 'for file in \"$@\"; do case \"$file\" in gen/*|vendor/*|*/gen/*|*/vendor/*) ;; *) gofmt -w \"$file\" ;; esac; done' --"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 40 - 42, Update the exclusion pattern used in package.json's gofmt pre-commit hook: the current case pattern "*/gen/*|*/vendor/*" skips nested gen/vendor dirs but not top-level "gen/" or "vendor/". Modify the case pattern in the sh -c command (the string containing "for file in \"$@\"; do case \"$file\" in ... esac; done") to also match top-level directories (e.g., add alternatives for "gen/*" and "vendor/*" or use a combined pattern that covers both top-level and nested paths) so top-level generated/vendor files are skipped by gofmt.controlplane/package.json (1)
29-30: Simplifyprettiercommand by removing redundant--checkflag.The
--writeand--checkflags together produce confusing behavior: while both run,--writesuppresses the exit code failure that--checknormally triggers, making--checkeffectively redundant in an autofix script. Useprettier -w .for clarity.Suggested fix
- "format": "prettier -w -c .", + "format": "prettier -w .",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/package.json` around lines 29 - 30, The package.json "format" script uses both prettier -w and -c together which is redundant and confusing; update the "format" script entry referenced as "format" in package.json to remove the --check/-c flag so it runs simply as prettier -w . (leave the "lint:fix" script unchanged other than relying on the updated "format" script).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CONTRIBUTING.md`:
- Line 68: Update the wording that currently refers to "pre-commit hooks" to use
a more accurate term like "Git hooks" and clarify that commit message validation
is handled by the commit-msg hook; specifically, edit the sentence containing
the phrase "Run `pnpm husky` to install pre-commit hooks which format code and
check commit message format" so it refers to installing Git hooks (or explicitly
mentions both pre-commit and commit-msg hooks) and keeps the `pnpm husky`
command intact.
- Around line 179-180: Update the two bullet lines to use the official casing:
change "the GitHub rest API" to "the GitHub REST API" and change "the github
artifact" to "the GitHub artifact" so both bullets consistently use correct
GitHub/REST capitalization.
In `@Makefile`:
- Around line 58-70: The format target currently accepts zero or multiple
package args and the GENERIC no-op targets (FORMAT_ARGS / .PHONY) swallow extra
goals; change the validation so the format recipe enforces exactly one package
argument and fails fast otherwise. In the format target use the computed package
variable and check the count of FORMAT_ARGS (or words after the "format" goal)
is exactly 1, printing a usage message and exiting non-zero if not; remove or
adjust the generic no-op target creation that defines $(FORMAT_ARGS): @: so
extra names do not become silent no-ops and ensure pnpm --filter "./$$package"
--fail-if-no-match run format only runs when exactly one package is provided.
In `@package.json`:
- Line 26: The workspace "format" script currently runs "pnpm run -r --parallel
format" which fails if some packages lack a format script; update the script
value to include the --if-present flag (i.e., run "pnpm run -r --parallel
--if-present format") so pnpm will skip packages without a format script; edit
the "format" entry in package.json to add --if-present while keeping the
recursive and parallel flags intact.
---
Duplicate comments:
In `@cli/package.json`:
- Around line 31-32: The "format" npm script currently mixes write and check
flags ("prettier -w -c ."), which conflicts when invoked from "lint:fix"; update
the "format" script to write-only by removing the check flag so it becomes a
pure write operation (keep the -w/--write and the target glob), ensuring
"lint:fix" can run "pnpm format" without flag conflicts; target the "format"
script entry in package.json and modify it accordingly.
In `@composition/package.json`:
- Around line 23-24: The "lint:fix" script currently calls "pnpm format" which
maps to the "format" script using "prettier -w -c .", so fix mode is being mixed
with check mode; update the "format" script to remove the check flag (change
"prettier -w -c ." to "prettier -w .") so "lint:fix" runs a write-only
formatter, or alternatively change "lint:fix" to directly call a write-only
Prettier (e.g., "prettier -w ."); optionally add a separate "format:check"
script if you still need a check-only command.
In `@playground/package.json`:
- Line 37: The "format" npm script currently uses conflicting Prettier flags
("-w" and "-c") in the "format" script entry; change it to a write-only command
by removing the check flag (use "prettier -w ." or the equivalent long form
"--write .") or split into two scripts (e.g., "format" with --write and
"format:check" with --check) so "format" no longer passes both --write and
--check simultaneously; update the "format" script name in package.json
accordingly.
In `@shared/package.json`:
- Around line 22-24: The format script currently mixes write and check flags;
update the "format" npm script referenced by "lint:fix" to run Prettier in
write-only mode (remove the check flag) so formatting is deterministic—replace
the current "prettier -w -c ." invocation with a write-only invocation (e.g.,
use --write or -w only) in package.json's "format" script and keep "lint:fix"
delegating to "pnpm format".
---
Nitpick comments:
In `@CONTRIBUTING.md`:
- Line 69: Update the sentence in CONTRIBUTING.md to improve grammar by
replacing “In some setup,” with a clearer phrase such as “In some setups,” (or
“In some environments,”) so the note reads: “In some setups, you have to tell
husky where to find your package manager or binaries.” Locate the sentence in
the husky installation note and apply the replacement to ensure subject-verb
agreement and clarity.
In `@controlplane/package.json`:
- Around line 29-30: The package.json "format" script uses both prettier -w and
-c together which is redundant and confusing; update the "format" script entry
referenced as "format" in package.json to remove the --check/-c flag so it runs
simply as prettier -w . (leave the "lint:fix" script unchanged other than
relying on the updated "format" script).
In `@package.json`:
- Around line 40-42: Update the exclusion pattern used in package.json's gofmt
pre-commit hook: the current case pattern "*/gen/*|*/vendor/*" skips nested
gen/vendor dirs but not top-level "gen/" or "vendor/". Modify the case pattern
in the sh -c command (the string containing "for file in \"$@\"; do case
\"$file\" in ... esac; done") to also match top-level directories (e.g., add
alternatives for "gen/*" and "vendor/*" or use a combined pattern that covers
both top-level and nested paths) so top-level generated/vendor files are skipped
by gofmt.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
.gitignore.husky/commit-msg.husky/pre-commitCONTRIBUTING.mdMakefileadmission-server/package.jsonaws-lambda-router/package.jsoncdn-server/package.jsoncli/package.jsonclient-tests/apollo-js/package.jsoncomposition/package.jsoncontrolplane/.prettierignorecontrolplane/package.jsongraphqlmetrics/package.jsonpackage.jsonplayground/package.jsonpnpm-workspace.yamlprotographic/package.jsonrouter-plugin/package.jsonrouter/package.jsonshared/package.jsonstudio/package.json
🚧 Files skipped from review as they are similar to previous changes (10)
- cdn-server/package.json
- controlplane/.prettierignore
- studio/package.json
- .husky/commit-msg
- protographic/package.json
- .gitignore
- graphqlmetrics/package.json
- router/package.json
- .husky/pre-commit
- admission-server/package.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controlplane/test/feature-subgraph/update-feature-subgraph.test.ts (1)
191-191:⚠️ Potential issue | 🟡 MinorUnused
roleparameter in test callback.Same issue as line 151 - this test has
async (role) =>but it's not atest.each(). Theroleparameter is unused and will beundefined.🧹 Suggested fix
- test('subgraph-admin should be able to update allowed feature subgraph', async (role) => { + test('subgraph-admin should be able to update allowed feature subgraph', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/feature-subgraph/update-feature-subgraph.test.ts` at line 191, The test "subgraph-admin should be able to update allowed feature subgraph" currently declares an unused callback parameter `role` (async (role) =>) which will be undefined; remove the parameter so the callback is just async () =>. Locate the test by its description string and update the function signature (and any other tests with the same pattern, e.g. the similar case around line 151) to eliminate the unused `role` parameter.
🧹 Nitpick comments (9)
controlplane/test/cache-warmer/push-cache-operation.test.ts (1)
546-582: Ensure server cleanup runs even on assertion failures.In this test body,
server.close()is only reached on the happy path. Wrapping the test body intry/finallyavoids open-handle leaks when an assertion fails early.♻️ Proposed refactor
- async (role) => { - const { client, server, authenticator, users } = await SetupTest({ + async (role) => { + const { client, server, authenticator, users } = await SetupTest({ dbname, chClient, enableMultiUsers: true, setupBilling: { plan: 'enterprise', }, }); - - const federatedGraphName = genID('fedGraph'); - await createFederatedAndSubgraph(client, federatedGraphName); - - const configureCacheWarmerResp = await client.configureCacheWarmer({ - namespace: 'default', - enableCacheWarmer: true, - }); - expect(configureCacheWarmerResp.response?.code).toBe(EnumStatusCode.OK); - - (chClient.queryPromise as Mock).mockResolvedValue([]); - - const operationContent = 'query Hello { hello { message } }'; - - authenticator.changeUserWithSuppliedContext({ - ...users.adminAliceCompanyA, - rbac: createTestRBACEvaluator(createTestGroup({ role })), - }); - - const pushCacheOperationResp = await client.pushCacheWarmerOperation({ - federatedGraphName, - namespace: 'default', - operationName: 'Hello', - operationContent, - }); - expect(pushCacheOperationResp.response?.code).toBe(EnumStatusCode.OK); - - await server.close(); + try { + const federatedGraphName = genID('fedGraph'); + await createFederatedAndSubgraph(client, federatedGraphName); + + const configureCacheWarmerResp = await client.configureCacheWarmer({ + namespace: 'default', + enableCacheWarmer: true, + }); + expect(configureCacheWarmerResp.response?.code).toBe(EnumStatusCode.OK); + + (chClient.queryPromise as Mock).mockResolvedValue([]); + + const operationContent = 'query Hello { hello { message } }'; + + authenticator.changeUserWithSuppliedContext({ + ...users.adminAliceCompanyA, + rbac: createTestRBACEvaluator(createTestGroup({ role })), + }); + + const pushCacheOperationResp = await client.pushCacheWarmerOperation({ + federatedGraphName, + namespace: 'default', + operationName: 'Hello', + operationContent, + }); + expect(pushCacheOperationResp.response?.code).toBe(EnumStatusCode.OK); + } finally { + await server.close(); + } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/cache-warmer/push-cache-operation.test.ts` around lines 546 - 582, The test does not guarantee server cleanup on assertion failures; wrap the main test logic (the call to SetupTest and all subsequent actions including calls to createFederatedAndSubgraph, client.configureCacheWarmer, client.pushCacheWarmerOperation, and the mocked chClient.queryPromise) in a try/finally and call server.close() inside the finally block so the server is always closed even if any expectation (expect(...).toBe(...)) throws; refer to the SetupTest result (client, server, authenticator, users) and ensure server.close() is invoked from finally.controlplane/test/feature-subgraph/get-feature-subgraphs.test.ts (1)
190-266: Protectserver.close()withfinallyin parameterized tests.If an assertion fails before Line 265, teardown is skipped for that case. Wrapping the body in
try/finallymakes cleanup deterministic and avoids cross-test leakage.Suggested refactor
test.each(['subgraph-admin', 'subgraph-publisher', 'subgraph-viewer'])( '%s should be able to list feature subgraphs from allowed namespaces', async (role) => { - const { client, server, authenticator, users } = await SetupTest({ dbname }); + const { client, server, authenticator, users } = await SetupTest({ dbname }); + try { const subgraphName = genID('subgraph'); const featureSubgraphName = genID('featureSubgraph'); const flagName = genID('flag'); @@ expect(listFeatureSubgraphsResp.response?.code).toBe(EnumStatusCode.OK); expect(listFeatureSubgraphsResp.count).toBe(1); - await server.close(); + } finally { + await server.close(); + } }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/feature-subgraph/get-feature-subgraphs.test.ts` around lines 190 - 266, Wrap the test body for the parameterized async (role) => { ... } handler in a try/finally so server.close() always runs; specifically, after calling SetupTest(...) and running assertions within the try block, move the await server.close() into a finally block and guard it (if (server) await server.close()) to avoid leaking the server when assertions throw; update the async handler that uses SetupTest, client, server, authenticator to use try { ... } finally { if (server) await server.close(); }.controlplane/test/feature-flag/delete-feature-flag.test.ts (1)
85-117: Usetry/finallyto guaranteeserver.close()on failures.If any assertion throws before the last line, the server is left open and can make later tests flaky.
♻️ Suggested resilient cleanup pattern
test.each(['organization-admin', 'organization-developer'])( '%s should be able to delete feature flag', async (role) => { const { client, server, users, authenticator } = await SetupTest({ dbname }); + try { - const subgraphName = genID('subgraph'); - const featureSubgraphName = genID('featureSubgraph'); + const subgraphName = genID('subgraph'); + const featureSubgraphName = genID('featureSubgraph'); - await createBaseAndFeatureSubgraph( - client, - subgraphName, - featureSubgraphName, - DEFAULT_SUBGRAPH_URL_ONE, - DEFAULT_SUBGRAPH_URL_TWO, - ); + await createBaseAndFeatureSubgraph( + client, + subgraphName, + featureSubgraphName, + DEFAULT_SUBGRAPH_URL_ONE, + DEFAULT_SUBGRAPH_URL_TWO, + ); - const featureFlagName = genID('flag'); - await createFeatureFlag(client, featureFlagName, [], [featureSubgraphName]); + const featureFlagName = genID('flag'); + await createFeatureFlag(client, featureFlagName, [], [featureSubgraphName]); - authenticator.changeUserWithSuppliedContext({ - ...users.adminAliceCompanyA, - rbac: createTestRBACEvaluator(createTestGroup({ role })), - }); + authenticator.changeUserWithSuppliedContext({ + ...users.adminAliceCompanyA, + rbac: createTestRBACEvaluator(createTestGroup({ role })), + }); - const deleteFeatureFlagResponseOne = await client.deleteFeatureFlag({ name: featureFlagName }); - expect(deleteFeatureFlagResponseOne.response?.code).toBe(EnumStatusCode.OK); + const deleteFeatureFlagResponseOne = await client.deleteFeatureFlag({ name: featureFlagName }); + expect(deleteFeatureFlagResponseOne.response?.code).toBe(EnumStatusCode.OK); - // attempting to delete the feature flag again should result in a not found error - const deleteFeatureFlagResponseTwo = await client.deleteFeatureFlag({ name: featureFlagName }); - expect(deleteFeatureFlagResponseTwo.response?.code).toBe(EnumStatusCode.ERR_NOT_FOUND); - expect(deleteFeatureFlagResponseTwo.response?.details).toBe( - `The feature flag "${featureFlagName}" was not found.`, - ); - - await server.close(); + // attempting to delete the feature flag again should result in a not found error + const deleteFeatureFlagResponseTwo = await client.deleteFeatureFlag({ name: featureFlagName }); + expect(deleteFeatureFlagResponseTwo.response?.code).toBe(EnumStatusCode.ERR_NOT_FOUND); + expect(deleteFeatureFlagResponseTwo.response?.details).toBe( + `The feature flag "${featureFlagName}" was not found.`, + ); + } finally { + await server.close(); + } }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/feature-flag/delete-feature-flag.test.ts` around lines 85 - 117, The test leaves the server running if an assertion throws; wrap the test logic that uses the server (the SetupTest call returning { client, server, users, authenticator } and subsequent operations like createBaseAndFeatureSubgraph, createFeatureFlag, authenticator.changeUserWithSuppliedContext, and the deleteFeatureFlag assertions) in a try/finally and call await server.close() in the finally block so the server is always closed even on failures.controlplane/test/feature-subgraph/update-feature-subgraph.test.ts (1)
13-13: Unused import:createSubgraph
createSubgraphis imported but not used anywhere in this file. Consider removing it.🧹 Suggested fix
import { createBaseAndFeatureSubgraph, createNamespace, - createSubgraph, DEFAULT_NAMESPACE, DEFAULT_SUBGRAPH_URL_ONE, DEFAULT_SUBGRAPH_URL_THREE, DEFAULT_SUBGRAPH_URL_TWO, SetupTest, } from '../test-util.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/feature-subgraph/update-feature-subgraph.test.ts` at line 13, The import createSubgraph is unused in update-feature-subgraph.test.ts; remove createSubgraph from the import list to eliminate the unused-import warning and keep imports minimal—locate the import statement that includes createSubgraph and delete just that identifier (or the whole import if it becomes empty) so other tests remain unchanged.controlplane/test/federated-graph/version.test.ts (1)
54-88: Usetry/finallyfor server teardown in each test.If any assertion fails before the last line,
server.close()is skipped. Wrapping test bodies intry/finallymakes teardown deterministic and prevents resource leakage/flaky follow-up tests.♻️ Suggested pattern
- const { client, server, authenticator, users } = await SetupTest({ dbname, chClient }); - const namespace = genID('namespace').toLowerCase(); - await createNamespace(client, namespace); - // ... - await server.close(); + const { client, server, authenticator, users } = await SetupTest({ dbname, chClient }); + try { + const namespace = genID('namespace').toLowerCase(); + await createNamespace(client, namespace); + // ... + } finally { + await server.close(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/federated-graph/version.test.ts` around lines 54 - 88, Wrap the test body produced by SetupTest so that server.close() is always called in a finally block: after awaiting SetupTest(...) and before running assertions (including calls to createNamespace, createThenPublishSubgraph, createFederatedGraph, authenticator.changeUserWithSuppliedContext, and client.getFederatedGraphByName), put the test logic inside try { ... } and call await server.close() inside finally { ... } to ensure deterministic teardown even if assertions fail; reference the SetupTest helper and the server variable to locate where to add the try/finally.controlplane/test/feature-flag/update-feature-flag.test.ts (2)
196-198: Fix local variable typo in map callback.Line 196 uses
featureSubraph; renaming tofeatureSubgraphimproves readability and consistency with nearby tests.Proposed patch
- expect(getFeatureFlagResponse.featureSubgraphs.map((featureSubraph) => featureSubraph.name)).toStrictEqual([ + expect(getFeatureFlagResponse.featureSubgraphs.map((featureSubgraph) => featureSubgraph.name)).toStrictEqual([ featureSubgraphNameTwo, ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/feature-flag/update-feature-flag.test.ts` around lines 196 - 198, The map callback in the assertion uses a misspelled local variable name `featureSubraph`; update the callback to use `featureSubgraph` so it matches the plural property and nearby tests—i.e., change the mapping in the expression that reads getFeatureFlagResponse.featureSubgraphs.map((featureSubraph) => featureSubraph.name) to use (featureSubgraph) => featureSubgraph.name and keep the rest of the assertion comparing to featureSubgraphNameTwo.
52-54: UseDEFAULT_NAMESPACEin the expected error string.Line 53 hardcodes
"default"even thoughDEFAULT_NAMESPACEis already imported. Using the constant makes this assertion resilient to namespace-default changes.Proposed patch
- expect(updateFeatureFlagResponse.response?.details).toBe( - `The feature flag "${featureFlagName}" does not exist in the namespace "default".`, - ); + expect(updateFeatureFlagResponse.response?.details).toBe( + `The feature flag "${featureFlagName}" does not exist in the namespace "${DEFAULT_NAMESPACE}".`, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/feature-flag/update-feature-flag.test.ts` around lines 52 - 54, The test assertion hardcodes the namespace string "default" instead of using the imported constant; update the expected error message to interpolate DEFAULT_NAMESPACE (used with featureFlagName and updateFeatureFlagResponse.response?.details) so the assertion reads the namespace from DEFAULT_NAMESPACE rather than the literal "default".controlplane/test/monograph/version.test.ts (1)
43-45: Differentiate duplicated test titles for clearer CI failure output.Both parameterized blocks currently emit identical titles for
graph-admin/graph-viewer, which makes failed-case attribution harder.♻️ Suggested title tweak
- test.each(['graph-admin', 'graph-viewer'])('%s should be able to read the version of a monograph', async (role) => { + test.each(['graph-admin', 'graph-viewer'])( + '%s should be able to read the version of a monograph on allowed namespace', + async (role) => {Also applies to: 85-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/monograph/version.test.ts` around lines 43 - 45, The two parameterized test.each blocks that currently use the identical title '%s should be able to read the version of a monograph' are producing duplicate test names for different parameter sets (e.g., graph-admin/graph-viewer); update the test titles in both test.each blocks to include the role parameter (e.g., change the title to include the role placeholder like '%s - should be able to read the version of a monograph' or 'role=%s: should be able to read...') so each invocation (in the test.each calls) yields a unique name for clearer CI failure output — locate the test.each calls and their title strings to make the small title change.controlplane/emails/package.json (1)
11-11: Add.tsxto ESLint extensions for complete email template coverage.The package contains five
.tsxfiles that are currently excluded from linting:
src/components/cosmo-logo.tsxsrc/components/main-container.tsxsrc/components/tailwind-config.tsxsrc/organizationDeletionQueued.tsxsrc/organizationInvite.tsxUpdate
--ext .ts,.mjs,.cjsto--ext .ts,.tsx,.mjs,.cjsto ensure these email templates are linted and formatted consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/emails/package.json` at line 11, Update the "lint:fix" npm script so ESLint and the formatter include .tsx files: modify the script value for "lint:fix" (the existing "--ext .ts,.mjs,.cjs" entry in package.json) to add ".tsx" (i.e. "--ext .ts,.tsx,.mjs,.cjs") so the email template files like src/components/cosmo-logo.tsx, src/components/main-container.tsx, src/components/tailwind-config.tsx, src/organizationDeletionQueued.tsx, and src/organizationInvite.tsx are linted and formatted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composition/README.md`:
- Around line 161-162: Update the contributing guidance sentence that currently
reads "When adding or changing error" to use the plural form for clarity; change
that fragment to "When adding or changing errors" (or "When adding or changing
error messages") so the README's sentence is grammatically correct and
unambiguous — locate the exact phrase in the README.md and replace it
accordingly.
In `@controlplane/emails/package.json`:
- Around line 10-11: The package.json script "format" currently combines
Prettier's write and check flags ("prettier -w -c ."), which defeats the check;
split into two scripts such as "format:fix": "prettier --write ." and
"format:check": "prettier --check .", replace "format" with one of those or keep
a wrapper if desired, and update the "lint:fix" script to invoke the fixer
(e.g., call "pnpm format:fix" instead of "pnpm format") so formatting and
format-check are separate and behave correctly; refer to the "format" and
"lint:fix" script names in package.json when making the edits.
In `@controlplane/test/feature-subgraph/update-feature-subgraph.test.ts`:
- Line 151: The test callback for "subgraph-admin should be able to update
feature subgraph from allowed namespaces" declares an unused parameter (async
(role) => ...) leftover from refactoring; remove the unused "role" parameter
from the test callback signature (i.e., change to async () => ...) so the
function signature matches how the test is invoked and avoids an undefined
variable, or if you intended parameterized tests, convert this test to test.each
and supply the roles accordingly.
In `@controlplane/test/federated-graph/version.test.ts`:
- Around line 102-104: The assertion for the namespace lookup is ineffective
because expect(getNamespaceResponse.response?.code) has no matcher; update the
test to assert an explicit expectation on getNamespaceResponse from
client.getNamespace (e.g., assert response.code equals the success code or is
defined). Locate the getNamespaceResponse variable and replace the bare expect
call with a concrete matcher such as asserting
getNamespaceResponse.response?.code is defined or equals the expected numeric
success value to properly validate the namespace lookup.
---
Outside diff comments:
In `@controlplane/test/feature-subgraph/update-feature-subgraph.test.ts`:
- Line 191: The test "subgraph-admin should be able to update allowed feature
subgraph" currently declares an unused callback parameter `role` (async (role)
=>) which will be undefined; remove the parameter so the callback is just async
() =>. Locate the test by its description string and update the function
signature (and any other tests with the same pattern, e.g. the similar case
around line 151) to eliminate the unused `role` parameter.
---
Nitpick comments:
In `@controlplane/emails/package.json`:
- Line 11: Update the "lint:fix" npm script so ESLint and the formatter include
.tsx files: modify the script value for "lint:fix" (the existing "--ext
.ts,.mjs,.cjs" entry in package.json) to add ".tsx" (i.e. "--ext
.ts,.tsx,.mjs,.cjs") so the email template files like
src/components/cosmo-logo.tsx, src/components/main-container.tsx,
src/components/tailwind-config.tsx, src/organizationDeletionQueued.tsx, and
src/organizationInvite.tsx are linted and formatted.
In `@controlplane/test/cache-warmer/push-cache-operation.test.ts`:
- Around line 546-582: The test does not guarantee server cleanup on assertion
failures; wrap the main test logic (the call to SetupTest and all subsequent
actions including calls to createFederatedAndSubgraph,
client.configureCacheWarmer, client.pushCacheWarmerOperation, and the mocked
chClient.queryPromise) in a try/finally and call server.close() inside the
finally block so the server is always closed even if any expectation
(expect(...).toBe(...)) throws; refer to the SetupTest result (client, server,
authenticator, users) and ensure server.close() is invoked from finally.
In `@controlplane/test/feature-flag/delete-feature-flag.test.ts`:
- Around line 85-117: The test leaves the server running if an assertion throws;
wrap the test logic that uses the server (the SetupTest call returning { client,
server, users, authenticator } and subsequent operations like
createBaseAndFeatureSubgraph, createFeatureFlag,
authenticator.changeUserWithSuppliedContext, and the deleteFeatureFlag
assertions) in a try/finally and call await server.close() in the finally block
so the server is always closed even on failures.
In `@controlplane/test/feature-flag/update-feature-flag.test.ts`:
- Around line 196-198: The map callback in the assertion uses a misspelled local
variable name `featureSubraph`; update the callback to use `featureSubgraph` so
it matches the plural property and nearby tests—i.e., change the mapping in the
expression that reads
getFeatureFlagResponse.featureSubgraphs.map((featureSubraph) =>
featureSubraph.name) to use (featureSubgraph) => featureSubgraph.name and keep
the rest of the assertion comparing to featureSubgraphNameTwo.
- Around line 52-54: The test assertion hardcodes the namespace string "default"
instead of using the imported constant; update the expected error message to
interpolate DEFAULT_NAMESPACE (used with featureFlagName and
updateFeatureFlagResponse.response?.details) so the assertion reads the
namespace from DEFAULT_NAMESPACE rather than the literal "default".
In `@controlplane/test/feature-subgraph/get-feature-subgraphs.test.ts`:
- Around line 190-266: Wrap the test body for the parameterized async (role) =>
{ ... } handler in a try/finally so server.close() always runs; specifically,
after calling SetupTest(...) and running assertions within the try block, move
the await server.close() into a finally block and guard it (if (server) await
server.close()) to avoid leaking the server when assertions throw; update the
async handler that uses SetupTest, client, server, authenticator to use try {
... } finally { if (server) await server.close(); }.
In `@controlplane/test/feature-subgraph/update-feature-subgraph.test.ts`:
- Line 13: The import createSubgraph is unused in
update-feature-subgraph.test.ts; remove createSubgraph from the import list to
eliminate the unused-import warning and keep imports minimal—locate the import
statement that includes createSubgraph and delete just that identifier (or the
whole import if it becomes empty) so other tests remain unchanged.
In `@controlplane/test/federated-graph/version.test.ts`:
- Around line 54-88: Wrap the test body produced by SetupTest so that
server.close() is always called in a finally block: after awaiting
SetupTest(...) and before running assertions (including calls to
createNamespace, createThenPublishSubgraph, createFederatedGraph,
authenticator.changeUserWithSuppliedContext, and
client.getFederatedGraphByName), put the test logic inside try { ... } and call
await server.close() inside finally { ... } to ensure deterministic teardown
even if assertions fail; reference the SetupTest helper and the server variable
to locate where to add the try/finally.
In `@controlplane/test/monograph/version.test.ts`:
- Around line 43-45: The two parameterized test.each blocks that currently use
the identical title '%s should be able to read the version of a monograph' are
producing duplicate test names for different parameter sets (e.g.,
graph-admin/graph-viewer); update the test titles in both test.each blocks to
include the role parameter (e.g., change the title to include the role
placeholder like '%s - should be able to read the version of a monograph' or
'role=%s: should be able to read...') so each invocation (in the test.each
calls) yields a unique name for clearer CI failure output — locate the test.each
calls and their title strings to make the small title change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (79)
.prettierignorecomposition/.prettierignorecomposition/CHANGELOG.mdcomposition/README.mdcomposition/tsconfig.jsoncomposition/vite.config.tscontrolplane/CHANGELOG.mdcontrolplane/README.mdcontrolplane/emails/package.jsoncontrolplane/emails/src/components/cosmo-logo.tsxcontrolplane/emails/src/components/main-container.tsxcontrolplane/emails/src/components/tailwind-config.tsxcontrolplane/emails/src/organizationDeletionQueued.tsxcontrolplane/emails/src/organizationInvite.tsxcontrolplane/src/templates/emails/organizationDeletionQueued.htmlcontrolplane/test/api-keys.test.tscontrolplane/test/apollo-federation.test.tscontrolplane/test/cache-warmer/configure-cache-warmer.test.tscontrolplane/test/cache-warmer/delete-cache-operation.test.tscontrolplane/test/cache-warmer/push-cache-operation.test.tscontrolplane/test/check-federated-graph.test.tscontrolplane/test/check-subgraph-schema.test.tscontrolplane/test/composition-warnings.test.tscontrolplane/test/delete-audit-logs.test.tscontrolplane/test/delete-user.test.tscontrolplane/test/feature-flag/create-feature-flag.test.tscontrolplane/test/feature-flag/delete-feature-flag.test.tscontrolplane/test/feature-flag/update-feature-flag.test.tscontrolplane/test/feature-subgraph/delete-feature-subgraph.test.tscontrolplane/test/feature-subgraph/get-feature-subgraph.test.tscontrolplane/test/feature-subgraph/get-feature-subgraphs.test.tscontrolplane/test/feature-subgraph/move-feature-subgraph.test.tscontrolplane/test/feature-subgraph/publish-feature-subgraph.test.tscontrolplane/test/feature-subgraph/update-feature-subgraph.test.tscontrolplane/test/federated-graph.test.tscontrolplane/test/federated-graph/version.test.tscontrolplane/test/graph-pruning.test.tscontrolplane/test/graphql/federationV1/pandas.graphqlcontrolplane/test/graphql/federationV1/products.graphqlcontrolplane/test/graphql/federationV1/users.graphqlcontrolplane/test/graphql/federationV2/products.graphqlcontrolplane/test/initialize-cosmo-user.test.tscontrolplane/test/invite-user.test.tscontrolplane/test/monograph/version.test.tscontrolplane/test/namespace/get-namespaces.test.tscontrolplane/test/oidc-provider.test.tscontrolplane/test/organization-groups.test.tscontrolplane/test/organization/create-organization.test.tscontrolplane/test/organization/getOrganizationBySlug.test.tscontrolplane/test/organization/leave-organization.test.tscontrolplane/test/organization/list-organizations.test.tscontrolplane/test/organization/remove-organization-member.test.tscontrolplane/test/proposal/proposal-webhooks.test.tscontrolplane/test/proposal/update-proposal.test.tscontrolplane/test/rbac-evaluator.test.tscontrolplane/test/restore-organization.test.tscontrolplane/test/router/compatibility-version/list.test.tscontrolplane/test/subgraph-check-extensions.test.tscontrolplane/test/subgraph/create-subgraph.test.tscontrolplane/test/subgraph/get-subgraphs.test.tscontrolplane/test/subgraph/move-subgraph.test.tscontrolplane/test/subgraph/publish-subgraph.test.tscontrolplane/test/subgraph/update-subgraph.test.tscontrolplane/test/test-data/contracts/products-include.graphqlcontrolplane/test/test-data/contracts/products-v2-includes.graphqlcontrolplane/test/test-data/contracts/products-v2.graphqlcontrolplane/test/test-data/contracts/products.graphqlcontrolplane/test/test-data/contracts/users-include.graphqlcontrolplane/test/test-data/contracts/users.graphqlcontrolplane/test/test-data/feature-flags/products-failing.graphqlcontrolplane/test/test-data/feature-flags/products-feature-update.graphqlcontrolplane/test/test-data/feature-flags/products-feature.graphqlcontrolplane/test/test-data/feature-flags/products.graphqlcontrolplane/test/test-data/feature-flags/users-feature.graphqlcontrolplane/test/test-data/feature-flags/users-update.graphqlcontrolplane/test/test-data/feature-flags/users.graphqlcontrolplane/test/test-data/plugin/service.proto.lock.jsoncontrolplane/test/utils.test.tscontrolplane/tsconfig.test.json
💤 Files with no reviewable changes (1)
- .prettierignore
✅ Files skipped from review due to trivial changes (43)
- controlplane/test/cache-warmer/delete-cache-operation.test.ts
- controlplane/test/organization/getOrganizationBySlug.test.ts
- controlplane/README.md
- controlplane/test/invite-user.test.ts
- controlplane/test/organization/remove-organization-member.test.ts
- controlplane/test/subgraph/move-subgraph.test.ts
- controlplane/test/organization/leave-organization.test.ts
- controlplane/test/subgraph/publish-subgraph.test.ts
- controlplane/test/feature-subgraph/move-feature-subgraph.test.ts
- controlplane/test/cache-warmer/configure-cache-warmer.test.ts
- controlplane/test/api-keys.test.ts
- composition/vite.config.ts
- controlplane/test/subgraph/create-subgraph.test.ts
- composition/tsconfig.json
- controlplane/test/proposal/proposal-webhooks.test.ts
- controlplane/test/composition-warnings.test.ts
- controlplane/test/oidc-provider.test.ts
- controlplane/emails/src/organizationInvite.tsx
- controlplane/test/graphql/federationV1/pandas.graphql
- controlplane/test/test-data/contracts/products.graphql
- controlplane/test/graph-pruning.test.ts
- controlplane/test/feature-subgraph/get-feature-subgraph.test.ts
- controlplane/test/proposal/update-proposal.test.ts
- controlplane/src/templates/emails/organizationDeletionQueued.html
- controlplane/emails/src/components/main-container.tsx
- controlplane/test/test-data/contracts/users.graphql
- controlplane/test/test-data/contracts/products-v2.graphql
- controlplane/test/organization-groups.test.ts
- controlplane/test/organization/list-organizations.test.ts
- controlplane/test/router/compatibility-version/list.test.ts
- controlplane/test/test-data/contracts/products-include.graphql
- controlplane/test/graphql/federationV2/products.graphql
- controlplane/test/federated-graph.test.ts
- controlplane/test/test-data/feature-flags/users.graphql
- controlplane/test/test-data/contracts/users-include.graphql
- controlplane/test/test-data/feature-flags/users-update.graphql
- controlplane/test/test-data/plugin/service.proto.lock.json
- controlplane/emails/src/components/cosmo-logo.tsx
- controlplane/test/feature-flag/create-feature-flag.test.ts
- controlplane/test/restore-organization.test.ts
- controlplane/test/apollo-federation.test.ts
- controlplane/test/check-subgraph-schema.test.ts
- controlplane/test/subgraph-check-extensions.test.ts
5 tasks
dkorittki
reviewed
Mar 4, 2026
Contributor
There was a problem hiding this comment.
Thanks for the work :) Would it be possible to also add formatting as a Makefile target for Go projects` ? It's what Go devs use for most tasks. Something like
format:
go fmt ./...Affected Makefiles should be in router/, router-tests/, composition-go/ and graphqlmetrics/
dkorittki
reviewed
Mar 4, 2026
devsergiy
approved these changes
Mar 4, 2026
endigma
requested changes
Mar 4, 2026
5 tasks
StarpTech
requested changes
Mar 4, 2026
Contributor
StarpTech
left a comment
StarpTech
left a comment
There was a problem hiding this comment.
Hi, this is going to touch the history of all files. Can we just introduce without touching all files?
Contributor
Author
In this phase, it touches just the controlplane, see reasones below. I looked at this closely to see if I can avoid touching the files, but I'm not sure if it will be possible. There are several problems with current formatter:
So it's really just inconsistency. One of the reasons I'm introducing the We can either merge it with the changes we have, or I can remove the |
This reverts commit 72c3966.
endigma
approved these changes
Mar 4, 2026
This was referenced Mar 5, 2026
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Chores
Bug Fixes
Checklist
Adds consistent way of code formatting for both JS/TS packages and Go packages.
JS/TS is relying on prettier, it loads the common configuration from the root
config.
For Go, we're relying on standard
gofmtfeatures. Bothpnpmandmakecan beused. Examples:
pnpm --filter=./cli formator
make format routerAdditionally, one can run
make format-allto format the whole monorepo. Someadjustments might be needed, if we want to ignore certain folders/files, but
it's a good start.
There are also git hook templates which can be installed via
pnpm husky, inconjuction with
lint-staged, any files can be automatically reformattedbefore commiting. This is optional, you can use the hooks or not, it's
up to you.
One minor adjustment to
controlplane: running tests will no longer attemptto format code. This was a bit inconvenient as it would stop you from
running tests while you were working on them, if your code contained formatting
issues.
I removed
prettierdependency fromcontrolplane, as it was causing twodifferent versions of formatter to be installed. I think we should use just
a single version.
I had to run the formatter on
controlplaneto make it pass. Forstudioandother packages, I'll probably run them as separate fixes in other PRs.